Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: test cases to have empty objects and not nulls #26

Merged
merged 1 commit into from
Aug 5, 2020
Merged

fix: test cases to have empty objects and not nulls #26

merged 1 commit into from
Aug 5, 2020

Conversation

derberg
Copy link
Member

@derberg derberg commented Aug 5, 2020

Description

  • test cases to have empty objects and not nulls

Related issue(s)
Fixes asyncapi/parser-js#144
Fixes #25

@derberg
Copy link
Member Author

derberg commented Aug 5, 2020

2 test cases from this PR are a ? to me:

  • tests/asyncapi-2.0/Message Trait Object/invalid-missing-content-type.yaml
  • tests/asyncapi-2.0/Message Object/invalid-missing-contentType.yaml

contentType and defaultContentType fields are not required by the spec, so in my opinion these test cases should be removed, probably in another PR for clarity? as I think it would mean we also remove tests/asyncapi-2.0/Message Object/valid-using-defaultContentType.yaml and tests/asyncapi-2.0/Message Trait Object/valid-using-defaultContentType.yaml

@derberg derberg requested a review from jstoiko August 5, 2020 06:06
@postatum postatum self-requested a review August 5, 2020 06:39
Copy link
Contributor

@postatum postatum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Thanks for the PR!

@postatum
Copy link
Contributor

postatum commented Aug 5, 2020

2 test cases from this PR are a ? to me:

* `tests/asyncapi-2.0/Message Trait Object/invalid-missing-content-type.yaml`

* `tests/asyncapi-2.0/Message Object/invalid-missing-contentType.yaml`

contentType and defaultContentType fields are not required by the spec, so in my opinion these test cases should be removed, probably in another PR for clarity? as I think it would mean we also remove tests/asyncapi-2.0/Message Object/valid-using-defaultContentType.yaml and tests/asyncapi-2.0/Message Trait Object/valid-using-defaultContentType.yaml

I've added these tests because, for example, this section says:

The content type to use when encoding/decoding a message's payload. The value MUST be a specific media type (e.g. application/json). When omitted, the value MUST be the one specified on the defaultContentType field.

Thus I assumed that either contentType or defaultContentType must be specified.

Feel free to open an issue so we can discuss this further.

@fmvilas
Copy link
Member

fmvilas commented Aug 5, 2020

I think you found a gap in the spec, @postatum. There should be a default value for defaultContentType or otherwise make it required (which I don't like). Can you create an issue on asyncapi/asyncapi repo?

@postatum
Copy link
Contributor

postatum commented Aug 5, 2020

I think you found a gap in the spec, @postatum. There should be a default value for defaultContentType or otherwise make it required (which I don't like). Can you create an issue on asyncapi/asyncapi repo?

Done asyncapi/spec#421

@fmvilas
Copy link
Member

fmvilas commented Aug 5, 2020

Thanks, mate!

@derberg
Copy link
Member Author

derberg commented Aug 5, 2020

created this issue to followup on the discussion #27

@derberg derberg merged commit 68a841d into asyncapi:master Aug 5, 2020
@derberg derberg deleted the remove-nulls branch August 5, 2020 08:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

invalid test cases with empty object YAML empty objects are null in JSON and cause parser errors
3 participants